WIP - Sunny/pipelines - #1
Conversation
ajalon1
left a comment
There was a problem hiding this comment.
Some very nice work so far. I'm looking at this from a very high level, mostly in regards to:
- general implementation as an API client
- implementation patterns that dovetail with the overall CLI, or can be used to improve the overall CLI
Obviously not a comprehensive review.
The only thing I would actually require as a change would be to rename the command to "pipeline" and permit "pipelines" as an alias.
|
|
||
| func Cmd() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "pipelines", |
There was a problem hiding this comment.
| Use: "pipelines", | |
| Use: "pipeline", | |
| Aliases: []string{"pipelines"}, |
to keep things consistent in CLI help.
| @@ -0,0 +1,178 @@ | |||
| <!-- TOOD: remove this file, just to keep track internally while development is happening --> | |||
There was a problem hiding this comment.
I appreciate this reference here. These summary docs really help when reviewing large PRs like this.
|
|
||
| flags.Bind(cmd) | ||
| cmd.Flags().IntVar(&offset, "offset", 0, "Pagination offset") | ||
| cmd.Flags().IntVar(&limit, "limit", 0, "Maximum number of inputs to return") |
There was a problem hiding this comment.
I noticed limit default of 0 in a lot of the list commands. I would expect that all of the list commands have a defined default limit -- it's just good practice for any API client.
| if pipelineID == "" { | ||
| return errors.New("--pipeline is required") | ||
| } |
There was a problem hiding this comment.
You can use MarkFlagRequired() to not have to reimplement this yourself.
| // Flags holds the values backing the shared --pipeline / --scope / | ||
| // --version flags. Bind() registers them on a cobra command and | ||
| // Resolve(cmd) turns them into a (Scope, *version) pair via | ||
| // pipelines.ResolveScope. | ||
| type Flags struct { | ||
| PipelineID string | ||
| Scope string | ||
| Version int | ||
| } |
There was a problem hiding this comment.
I actually really like this pattern. I wonder if it's possible for this to be used more consistently throughout the pipeline commands.
| func TestHandleDeleteError_404IsSuppressed(t *testing.T) { | ||
| httpErr := &drapi.HTTPError{StatusCode: http.StatusNotFound, URL: "http://x/api/v2/pipelines/abc"} | ||
|
|
||
| err := handleDeleteError(httpErr, "abc") | ||
| assert.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
Why do you return an error on 404 for some commands, and suppress it for others?
|
|
||
| if d.VersionID != nil { | ||
| scope = "locked" | ||
| versionDisplay = "v" + strconv.Itoa(*d.VersionID) |
52f4751 to
e1089f7
Compare
…oss#522) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- Replace UUID IDs with 24-char MongoDB ObjectIds in help text, test fixtures, and docs (pipeline_id, dispatch_id, input_id, schedule_id, environment_id now use ObjectId format per pipelines-api commit 65c86cd) - Add internal/pipelines/Time type with flexible UnmarshalJSON to handle the API's bare UTC datetime strings (no timezone suffix) alongside standard RFC3339, fixing the time-parse crash on pipelines list/get - Update Graph structs: GraphNode.ID and GraphEdge.Source/Target are now int (not string), GraphPipeline drops ID, adds PythonVersion; add extra edge/node fields for full --output json pass-through - Fix demo-pipelines Taskfile target to use correct port 8100 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covalent is not available in the local port-forward environment, so environment create/update time out. Mirrors the existing SKIP_SCHEDULES flag. Both default to false; pass SKIP_ENVIRONMENTS=true to skip the 8 environment steps (total drops to 27 when combined with SKIP_SCHEDULES). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
API now returns RFC3339Nano timestamps with Z suffix, so the custom pipeline.Time wrapper is no longer needed. Replace all Time fields in pipeline.go with standard time.Time and delete time.go. Also default DEMO_SKIP_SCHEDULES and DEMO_SKIP_ENVIRONMENTS to true in the demo-pipelines Taskfile task (k8s/Covalent not available locally), and pipe demo output to output.txt via tee. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
API is now RFC3339 compliant; replace string datetime fields with time.Time across run, input, schedule, and environment structs. Update render helpers to format with .UTC().Format(time.RFC3339), consistent with the pipeline/version commands. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add cmd/pipelines/outputfmt: OutputFormat type + AddOutputFlag() with --output-format flag, mirroring internal/workload/flags.go - Add cmd/pipelines/pipelineutil: RenderPipeline/RenderPipelines with lipgloss/table list view and tabwriter detail view - Rewrite all *util/render.go files: add Render*() routing functions, replace tabwriter lists with lipgloss/table, switch to tabwriter for detail views, add DTOs for input/schedule/environment/version, adopt "2006-01-02 15:04 UTC" timestamp format - Update all 23 verb cmd files: use outputfmt.AddOutputFlag, remove scattered --output StringVar and manual validation, call Render*() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2e6cc30 to
3d77d0f
Compare
…pattern Consolidates all *util sibling packages (runutil, inpututil, scheduleutil, envutil, versionutil, pipelineutil, outputfmt) into internal/pipelines/, mirroring how internal/workload/ owns its own output helpers. Each cmd verb now imports only internal/pipelines and calls e.g. pipelines.RenderRuns(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-056 - camelCase all JSON tags to match DrBaseModel alias_generator (action 056) - Primary resource IDs now decode as "id" on the wire - Wrap all list endpoints in DataPage[T] envelope (action 056) - Remove PipelineName (lattice_name) from PipelineVersion — dropped by action 055 - Rename electron_names → taskNames, python_version → pythonVersion, etc. - Update all test mock responses and DataPage[ListItem] in list cmd tests - Fix pre-existing invalid "t" timestamps in environment_test.go Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Renames cmd/pipelines/ → cmd/pipeline/, updates the cobra Use field, feature gate key (DATAROBOT_CLI_FEATURE_PIPELINE), and all example strings. Adds "pipelines" as a backward-compat alias per naming conventions. Docs and demo.sh updated accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lient
Replace the duplicated inline token+header setup and &http.Client{}
construction in Get, Post, Patch, and Delete with the AuthorizeRequest
and NewHTTPClient helpers introduced in client.go. Also fixes a bug in
Post/Delete where config.GetAPIKey was called directly, bypassing the
--skip-auth flag. Migrates raw viper imports to viperx in get.go and
affected test files to satisfy depguard.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…k and header consistency - Move AuthorizeRequest from client.go to auth.go (its natural home); remove the now-redundant SetAuthHeaders wrapper and update filesapi callers to call AuthorizeRequest directly - Fix --skip-auth bypass: auth.go previously called config.GetAPIKey directly, bypassing resolveToken(); now all callers go through getToken() which honours skip_auth - Fix GetJSON body leak: use defer resp.Body.Close() so body is closed on decode error - Fix Header.Add → Header.Set for Content-Type in delete.go, post.go, patch.go - Replace hand-rolled fakeBody (missing io.EOF) with strings.NewReader in client_test.go Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pipelines-api action 059 renamed the graph JSON key from "lattice" to "pipeline" (PipelineGraphLattice → PipelineGraphMeta). Update the Go struct tag and test fixtures to match the new wire format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Renames the Go package from `internal/pipelines` to `internal/pipeline` to match the singular command name (`dr pipeline`) and updates all cmd imports, tests, and telemetry calls accordingly. Also adds telemetry TrackWith calls across all pipeline subcommands. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
RATIONALE
CHANGES
PR Automation
Comment-Commands: Trigger CI by commenting on the PR:
/trigger-smoke-testor/trigger-test-smoke- Run smoke tests/trigger-install-testor/trigger-test-install- Run installation testsLabels: Apply labels to trigger workflows:
run-smoke-testsorgo- Run smoke tests on demand (only works for non-forked PRs)Important
For Forked PRs: The
run-smoke-testslabel won't work. A required Smoke Tests check will block merge until a maintainer acts:/approve-smoke-teststo run smoke tests (results will set the check)/skip-smoke-teststo bypass the check without running testsPlease comment requesting a maintainer review if you need smoke tests to run.